packages/ciphernode/test_helpers/Cargo.toml (1)

16-16: LGTM! Consider adding features for tracing.

The addition of the tracing dependency is appropriate and aligns with the PR objectives to enhance logging capabilities. This change will allow the use of structured logging in the test helpers, which can significantly improve debugging and observability during testing.

Consider specifying any specific features of the tracing crate that you intend to use. For example, if you plan to use the log compatibility layer, you could modify the dependency like this:

tracing = { workspace = true, features = ["log"] }

This can help to explicitly document which features of tracing are being used in the test helpers.

packages/ciphernode/sortition/Cargo.toml (1)

18-18: LGTM! Consider specifying features for tracing.

The addition of the tracing crate as a workspace dependency is a good step towards improving logging capabilities, which aligns well with the PR objectives. This change will enable structured, contextual, and async-aware logging in the sortition package.

Consider specifying the features you intend to use from the tracing crate. This can help optimize compile times and binary size. For example:

tracing = { workspace = true, features = ["attributes"] }

Replace ["attributes"] with the specific features you need. If you're unsure which features to include, you can leave it as is for now and optimize later when you have a better understanding of your logging requirements.

packages/ciphernode/enclave/Cargo.toml (1)

17-18: LGTM! Consider adding version constraints.

The addition of tracing and tracing-subscriber dependencies aligns well with the PR objectives to enhance logging functionality. Using workspace = true ensures consistent versioning across the workspace, which is a good practice.

Consider adding version constraints to these dependencies in the workspace Cargo.toml file if they're not already present. This helps ensure reproducible builds and easier upgrades in the future.

packages/ciphernode/test_helpers/src/utils.rs (2)

12-15: LGTM: Good addition of structured logging with error handling.

The match statement effectively logs the path being written to and handles the case where the path can't be converted to a string. The use of trace! for successful cases and error! for failures is appropriate.

Consider using a consistent style for the path field in both arms of the match:

 match abs_path.to_str() {
     Some(s) => trace!(path = s, "Writing to path"),
-    None => error!(path=?abs_path, "Cannot parse path"),
+    None => error!(path = ?abs_path, "Cannot parse path"),
 }

This change adds a space after = in the None arm, matching the style in the Some arm.

---

Line range hint 1-28: Overall: Excellent implementation of structured logging.

The changes in this file successfully implement the PR objective of replacing println with tracing::xxxx() logging methods. The additions enhance the logging capabilities of the write_file_with_dirs function without altering its core functionality. These improvements will significantly aid in debugging and monitoring.

Key improvements:

1. Added appropriate imports for tracing.
2. Implemented structured logging for the file path being written.
3. Added error logging for cases where the path can't be parsed.
4. Replaced println! with trace! for successful file writes.

The suggested minor improvements (consistent formatting and safer path handling) will further enhance the robustness of the logging implementation.

Consider applying similar logging improvements consistently across the codebase to maintain a uniform approach to logging and error handling.

packages/ciphernode/enclave/src/bin/aggregator.rs (2)

22-22: LGTM with suggestions: Tracing subscriber initialized

The tracing subscriber is correctly initialized, which is necessary for the tracing macros to function. However, consider the following suggestions:

1. Configure log levels, possibly based on environment (e.g., debug level for development, info for production).
2. Consider allowing configuration of output destinations (e.g., stdout, file) based on runtime parameters.

Example:

let subscriber = tracing_subscriber::FmtSubscriber::builder()
    .with_max_level(tracing::Level::INFO)
    .finish();
tracing::subscriber::set_global_default(subscriber).expect("setting default subscriber failed");

---

Line range hint 1-33: Summary: Tracing successfully implemented

The changes in this file successfully implement the tracing crate for improved logging:

1. The necessary import for tracing::info has been added.
2. The tracing subscriber is initialized, enabling the use of tracing macros.
3. The println! macro has been replaced with the info! macro from tracing.

These changes align well with the PR objectives and enhance the logging capabilities of the application. Consider the earlier suggestion about configuring log levels and output destinations for even more flexibility.

To further improve the logging architecture:

1. Consider creating a separate logging module to centralize logging configuration.
2. Implement different log levels (debug, warn, error) throughout the application for more granular logging.
3. Add context to log messages where appropriate, such as including relevant variable values or system state.

packages/ciphernode/test_helpers/src/public_key_writer.rs (2)

32-32: LGTM: Improved logging with structured format.

The replacement of println! with info! enhances the logging mechanism by using structured logging. This change aligns well with the PR objectives and improves the overall logging quality.

Consider adding more context to the log message:

info!(path = &self.path, "Writing aggregated public key to file");

This provides slightly more information about what type of public key is being written.

---

Line range hint 1-34: Overall: Good improvement in logging, consider extending throughout the file.

The changes successfully implement structured logging using the tracing crate, which aligns with the PR objectives. This improves log quality and parseability without affecting the core functionality of the PublicKeyWriter.

For consistency, consider updating other potential logging statements in this file (if any) to use the tracing crate. Also, it might be beneficial to add debug-level logging for other significant operations in the PublicKeyWriter, such as when it's attached to the event bus.

Example:

pub fn attach(path: &str, bus: Addr<EventBus>) -> Addr<Self> {
    let addr = Self {
        path: path.to_owned(),
    }
    .start();
    bus.do_send(Subscribe {
        listener: addr.clone().recipient(),
        event_type: "PublicKeyAggregated".to_string(),
    });
    debug!(path = path, "PublicKeyWriter attached to event bus");
    addr
}

This would provide a more comprehensive logging coverage for the PublicKeyWriter operations.

packages/ciphernode/test_helpers/src/plaintext_writer.rs (1)

34-34: LGTM: Improved logging with structured fields.

The replacement of println! with info! successfully implements the PR's objective. The use of a structured field (path) enhances the log's informativeness, making it easier to filter and analyze logs.

A minor suggestion for improvement:

Consider adding the output.len() to the log message to provide more context. This could be helpful for debugging or monitoring purposes. Here's a suggested modification:

info!(path = &self.path, output_length = output.len(), "Writing Plaintext To Path");

packages/ciphernode/enclave/src/main.rs (2)

33-33: LGTM: Tracing subscriber initialized, consider error handling

The initialization of the tracing subscriber is correctly placed at the beginning of the main function. This sets up the logging infrastructure as intended.

Consider adding error handling for the initialization:

tracing_subscriber::fmt::init().expect("Failed to initialize tracing");

This will provide more informative feedback if the initialization fails.

---

38-38: LGTM: Println replaced with info! macro

The replacement of println! with info! for logging the ciphernode launch is correct and aligns with the PR objective. This change enhances logging by using structured logging.

For consistency with typical log messages, consider changing the log message to lowercase:

info!("Launching ciphernode: ({})", address);

This follows common logging conventions where only the first letter is capitalized.

packages/ciphernode/router/src/ciphernode_selector.rs (2)

55-55: Consider using into() instead of clone().

The change to clone address when passing it to GetHasNode is good for ownership. However, for a small optimization, consider using address.into() instead of address.clone(). This allows for a potentially more efficient conversion if address is already owned.

-                    address: address.clone(),
+                    address: address.into(),

---

61-61: LGTM: Informative log message added.

The addition of this log message aligns well with the PR objective of enhancing logging functionality. It provides valuable information about ciphernode selection, which can aid in debugging and monitoring.

Consider adding more context to the log message:

-                    info!(node = address, "Ciphernode was not selected");
+                    info!(node = address, seed = seed, size = size, "Ciphernode was not selected for committee");

This change would provide more context about the selection process, including the seed and committee size.

packages/ciphernode/evm/src/helpers.rs (1)

35-41: LGTM: Logging statements added effectively

The new logging statements enhance the observability of the stream_from_evm function without altering its core functionality. They provide valuable insights into the log processing flow.

Suggestion for improvement:
Consider adding more context to the log messages. For example:

-trace!("Received log from EVM");
+trace!("Received log from EVM: {:?}", log);

-trace!("Failed to extract log from EVM");
+trace!("Failed to extract log from EVM: {:?}", log);

-info!("Extracted log from evm sending now.");
+info!("Extracted log from EVM, sending event: {:?}", event);

This additional context could be valuable for debugging and monitoring.

packages/ciphernode/evm/src/enclave_sol_writer.rs (1)

97-97: Approved: Improved logging with structured format

The replacement of println! with info! and the use of a structured logging format is a significant improvement. This change aligns perfectly with the PR objective and enhances the logging mechanism.

Consider adding a more descriptive message to provide context:

-info!(tx=%receipt.transaction_hash, "tx")
+info!(tx=%receipt.transaction_hash, "Transaction published successfully")

This change would make the log message more informative and easier to understand at a glance.

packages/ciphernode/evm/src/registry_filter_sol.rs (1)

96-96: Approved: Enhanced logging with tracing

The replacement of println! with info! from the tracing crate is a good improvement. It provides structured logging, which is more suitable for production environments and easier to parse programmatically.

A minor suggestion to improve clarity:

Consider adding a more descriptive message to the log. For example:

info!(tx=%receipt.transaction_hash, "Committee published successfully");

This provides more context about what the transaction represents.

packages/ciphernode/router/src/hooks.rs (3)

40-42: LGTM: Improved error handling for LazyKeyshare

The addition of error handling for the missing fhe instance is a good improvement. It provides clear feedback when key generation cannot proceed due to missing dependencies.

Consider extracting the error message to a constant for easier maintenance and potential reuse:

const ERR_MSG_FHE_NOT_SET: &str = "Could not create Keyshare because the fhe instance it depends on was not set on the context.";

// Then use it in the error reporting:
bus.err(EnclaveErrorType::KeyGeneration, anyhow!(ERR_MSG_FHE_NOT_SET));

---

59-64: LGTM: Enhanced error handling for LazyPlaintextAggregator

The addition of error handling for missing fhe and meta instances is a good improvement. It provides clear feedback when plaintext aggregation cannot proceed due to missing dependencies.

For consistency with the previous suggestion, consider extracting the error messages to constants:

const ERR_MSG_FHE_NOT_SET_PLAINTEXT: &str = "Could not create PlaintextAggregator because the fhe instance it depends on was not set on the context.";
const ERR_MSG_META_NOT_SET_PLAINTEXT: &str = "Could not create PlaintextAggregator because the meta instance it depends on was not set on the context.";

// Then use them in the error reporting:
bus.err(EnclaveErrorType::PlaintextAggregation, anyhow!(ERR_MSG_FHE_NOT_SET_PLAINTEXT));
bus.err(EnclaveErrorType::PlaintextAggregation, anyhow!(ERR_MSG_META_NOT_SET_PLAINTEXT));

---

94-99: LGTM: Improved error handling for LazyPublicKeyAggregator

The addition of error handling for missing fhe and meta instances is a good improvement. It provides clear feedback when public key aggregation cannot proceed due to missing dependencies.

For consistency with the previous suggestions, consider extracting the error messages to constants:

const ERR_MSG_FHE_NOT_SET_PUBLICKEY: &str = "Could not create PublicKeyAggregator because the fhe instance it depends on was not set on the context.";
const ERR_MSG_META_NOT_SET_PUBLICKEY: &str = "Could not create PublicKeyAggregator because the meta instance it depends on was not set on the context.";

// Then use them in the error reporting:
bus.err(EnclaveErrorType::PublickeyAggregation, anyhow!(ERR_MSG_FHE_NOT_SET_PUBLICKEY));
bus.err(EnclaveErrorType::PublickeyAggregation, anyhow!(ERR_MSG_META_NOT_SET_PUBLICKEY));

Consider refactoring the error checking logic for fhe and meta instances into a separate function to reduce code duplication between LazyPlaintextAggregator::create and LazyPublicKeyAggregator::create. For example:

fn check_dependencies(ctx: &Context, bus: &Addr<EventBus>, error_type: EnclaveErrorType) -> Result<(), ()> {
    if ctx.fhe.is_none() {
        bus.err(error_type, anyhow!("Could not create aggregator because the fhe instance it depends on was not set on the context."));
        return Err(());
    }
    if ctx.meta.is_none() {
        bus.err(error_type, anyhow!("Could not create aggregator because the meta instance it depends on was not set on the context."));
        return Err(());
    }
    Ok(())
}

// Then use it in both create methods:
if check_dependencies(ctx, &bus, EnclaveErrorType::PlaintextAggregation).is_err() {
    return;
}

This refactoring would make the code more maintainable and reduce the risk of inconsistencies between similar error handling blocks.

packages/ciphernode/evm/src/enclave_sol_reader.rs (2)

59-61: Enhance error message for E3Requested parsing

The use of error! macro is appropriate for logging parsing errors. However, consider enhancing the error message to provide more context:

- error!("Error parsing event E3Requested after topic matched!");
+ error!("Failed to parse E3Requested event: {:?}", data);

This change would log the actual data that failed to parse, aiding in debugging efforts.

---

72-76: LGTM: Trace logging for unknown events (with minor typo)

The addition of trace! logging for unknown events is a good practice. It provides visibility into unexpected events without cluttering logs at higher severity levels.

There's a small typo in the log message:

- "Unknown event was received by Enclave.sol parser buut was ignored"
+ "Unknown event was received by Enclave.sol parser but was ignored"

packages/ciphernode/evm/src/ciphernode_registry_sol.rs (4)

11-11: LGTM! Consider grouping related imports.

The addition of the tracing crate import aligns well with the PR objective of enhancing logging functionality. The specific macros imported (error, info, trace) suggest a structured approach to logging at different levels, which is a good practice.

Consider grouping related imports together. You could move this import closer to other external crate imports for better organization.

---

73-75: LGTM! Consider adding more context to the error log.

The replacement of println! with the error! macro from tracing is a good improvement, aligning with the PR objectives. This change enhances the logging structure and will likely improve error tracking and debugging.

Consider adding more context to the error log, such as including the data that failed to parse. This could help with debugging in the future. For example:

error!("Error parsing event CiphernodeAdded after topic was matched. Data: {:?}", data);

---

81-83: LGTM! Consider adding more context to the error log.

The replacement of println! with the error! macro from tracing is consistent with the previous change and aligns well with the PR objectives. This change contributes to a more structured and effective logging system.

Similar to the previous suggestion, consider adding more context to the error log by including the data that failed to parse. For example:

error!("Error parsing event CiphernodeRemoved after topic was matched. Data: {:?}", data);

---

87-91: LGTM! Minor typo in the log message.

The addition of the trace! macro for logging unknown events is a good improvement. It enhances the traceability of event handling without cluttering logs with potentially frequent occurrences of unknown events. The use of trace! level is appropriate for this kind of information.

There's a minor typo in the log message. "buut" should be "but":

trace!(
    topic=?_topic,
    "Unknown event was received by Enclave.sol parser but was ignored"
);

tests/basic_integration/test.sh (2)

15-15: LGTM! Consider making the log level configurable.

The addition of export RUST_LOG=info aligns well with the PR objectives of enhancing logging functionality. This will enable more detailed logging for Rust components during test execution.

To improve flexibility, consider making the log level configurable:

-export RUST_LOG=info
+export RUST_LOG=${RUST_LOG:-info}

This change allows overriding the log level when running the script, while maintaining "info" as the default.

---

Line range hint 1-168: Consider adding bash logging statements for improved debugging.

While the RUST_LOG environment variable has been set for Rust components, adding some bash logging statements throughout this script could further enhance debugging capabilities during integration testing.

Here's an example of how you could add logging to key sections of the script:

log_info() {
    echo "[INFO] $(date '+%Y-%m-%d %H:%M:%S') - $1"
}

# Add this function near the top of the script

# Then use it throughout the script, for example:
log_info "Starting EVM node"
yarn evm:node &

log_info "Launching ciphernode $CIPHERNODE_ADDRESS_1"
yarn ciphernode:launch --address $CIPHERNODE_ADDRESS_1 --config "$SCRIPT_DIR/lib/ciphernode_config.yaml" &

# ... and so on for other key operations

This will provide a consistent logging format for bash operations, complementing the Rust logging.

🧰 Tools

🪛 Shellcheck

[warning] 18-18: RPC_URL appears unused. Verify use (or export if used externally).

(SC2034)

packages/ciphernode/aggregator/src/plaintext_aggregator.rs (3)

128-128: LGTM: Replaced println! with structured logging

The changes successfully replace println! statements with error! macros from the tracing crate, aligning with the PR objectives. This enhances the logging mechanism by providing structured logs instead of simple console output.

Minor suggestion: Consider adding more context to the error messages, such as including the e3_id in the log when the wrong ID is sent to the aggregator. This could further improve debuggability.

Example:

error!(?e3_id, expected_e3_id = ?act.e3_id, "Wrong e3_id sent to aggregator. This should not happen.");

Also applies to: 148-148, 153-153

---

Line range hint 64-64: LGTM: Improved add_share method return type

The change to return Result<PlaintextAggregatorState> instead of Result<()> is a good improvement. It provides more information about the aggregator's state after adding a share, allowing callers to react to state changes more effectively.

Suggestion: Consider using the ? operator for error propagation instead of early returns. This can make the code more concise and consistent with Rust's error handling patterns.

Example:

pub fn add_share(&mut self, share: Vec<u8>) -> Result<PlaintextAggregatorState> {
    let PlaintextAggregatorState::Collecting {
        threshold_m,
        shares,
        ciphertext_output,
        ..
    } = &mut self.state else {
        return Err(anyhow::anyhow!("Can only add share in Collecting state"));
    };

    shares.insert(share);
    Ok(if shares.len() == *threshold_m {
        PlaintextAggregatorState::Computing {
            shares: shares.clone(),
            ciphertext_output: ciphertext_output.to_vec(),
        }
    } else {
        self.state.clone()
    })
}

Also applies to: 65-65, 66-66, 67-67, 68-68, 69-69, 70-70, 71-71, 72-72, 73-73, 74-74, 75-75, 76-76, 77-77, 78-78, 79-79, 80-80, 81-81, 82-82, 83-83, 84-84

---

Line range hint 86-86: LGTM: Improved set_decryption method return type

The change to return Result<PlaintextAggregatorState> instead of Result<()> is consistent with the improvements made to the add_share method. This provides better visibility into the aggregator's state after setting the decryption.

Suggestion: For consistency with the add_share method, consider handling the case where the state is not Computing more explicitly:

pub fn set_decryption(&mut self, decrypted: Vec<u8>) -> Result<PlaintextAggregatorState> {
    let PlaintextAggregatorState::Computing { shares, .. } = &mut self.state else {
        return Err(anyhow::anyhow!("Can only set decryption in Computing state"));
    };

    let shares = shares.to_owned();
    Ok(PlaintextAggregatorState::Complete { decrypted, shares })
}

This change would make the error handling more explicit and consistent across both methods.

Also applies to: 87-87, 88-88, 89-89, 90-90, 91-91, 92-92, 93-93, 94-94, 95-95

packages/ciphernode/aggregator/src/publickey_aggregator.rs (4)

136-136: LGTM: Improved logging with structured format

The replacement of println! with error! enhances logging capabilities as intended. The inclusion of the state in the log message provides valuable context.

Consider adding a static string identifier to make log filtering easier:

error!(event = "aggregator_closed", state = ?self.state, "Aggregator has been closed for collecting keyshares.");

---

158-158: Approved: Logging improved, but could be more informative

The replacement of println! with error! is good. However, the log message could be more informative.

Consider adding more context:

error!(event = "node_not_in_committee", address = ?address, "Node not found in committee");

This addition provides more details about which node was not found, making debugging easier.

---

163-163: Approved: Logging improved, but could be more detailed

The switch to error! is good. However, for unexpected scenarios like this, more detailed logging would be beneficial.

Consider enhancing the log message:

error!(
    event = "wrong_e3_id",
    expected_e3_id = ?act.e3_id,
    received_e3_id = ?e3_id,
    "Incorrect e3_id sent to aggregator. This indicates a potential bug or security issue."
);

This provides more context about the mismatch and emphasizes the severity of the situation.

---

Line range hint 1-238: Summary: Logging improvements successfully implemented

The changes in this file successfully replace println! statements with tracing::error! calls, aligning with the PR objectives. These improvements enhance the ability to debug and monitor the application by providing structured logging.

Consider reviewing the entire file for other locations where similar logging improvements could be applied, especially for important events or error conditions. This would ensure consistency in logging practices throughout the PublicKeyAggregator implementation.

packages/ciphernode/core/src/events.rs (3)

186-201: Approve the new get_data method with a minor suggestion

The new get_data method provides a consistent way to get a string representation of the event data, which is excellent for logging and debugging purposes. It leverages the Display trait implementations of the inner data structures, promoting code reuse.

Consider handling the default case explicitly instead of commenting it out. You could either remove the comment or implement a default case:

 pub fn get_data(&self) -> String {
     match self.clone() {
         EnclaveEvent::KeyshareCreated { data, .. } => format!("{}", data),
         EnclaveEvent::E3Requested { data, .. } => format!("{}", data),
         EnclaveEvent::PublicKeyAggregated { data, .. } => format!("{}", data),
         EnclaveEvent::CiphertextOutputPublished { data, .. } => format!("{}", data),
         EnclaveEvent::DecryptionshareCreated { data, .. } => format!("{}", data),
         EnclaveEvent::PlaintextAggregated { data, .. } => format!("{}", data),
         EnclaveEvent::CiphernodeSelected { data, .. } => format!("{}", data),
         EnclaveEvent::CiphernodeAdded { data, .. } => format!("{}", data),
         EnclaveEvent::CiphernodeRemoved { data, .. } => format!("{}", data),
         EnclaveEvent::E3RequestComplete { data, .. } => format!("{}", data),
         EnclaveEvent::EnclaveError { data, .. } => format!("{:?}", data),
-        // _ => "<omitted>".to_string(),
+        _ => "<unknown event>".to_string(),
     }
 }

This ensures that all possible cases are handled explicitly.

---

330-334: Approve new Display implementations with a suggestion for consistency

The new Display implementations for various structs greatly enhance the readability of logs and debug outputs. They provide a consistent format for representing the data of different event types.

For consistency, consider using the same format for all structs. Some implementations use <omitted> for sensitive or large fields, while others simply exclude them. Standardize this approach across all implementations. For example:

 impl Display for PublicKeyAggregated {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         write!(
             f,
-            "e3_id: {}, src_chain_id: {}, nodes: <omitted>, pubkey: <omitted>",
-            self.e3_id, self.src_chain_id,
+            "e3_id: {}, src_chain_id: {}, nodes: <omitted>, pubkey: <omitted>",
+            self.e3_id, self.src_chain_id
         )
     }
 }

Apply this consistently to all Display implementations where applicable.

Also applies to: 344-348, 359-367, 379-386, 396-404, 413-417, 427-435, 444-448, 458-466, 476-484, 493-497, 502-506

---

376-377: Approve retention of src_chain_id with a suggestion for documentation

The decision to keep the src_chain_id field in the E3Requested struct has been finalized, as evidenced by the removal of comments indicating potential future modifications.

Consider adding a brief documentation comment to explain the purpose and significance of the src_chain_id field. This will help future developers understand its role in the E3 request process. For example:

 pub struct E3Requested {
     pub e3_id: E3id,
     pub threshold_m: usize,
     pub seed: Seed,
     pub params: Vec<u8>,
+    /// Identifier of the source chain initiating the E3 request
     pub src_chain_id: u64,
 }

packages/ciphernode/sortition/src/sortition.rs (1)

Line range hint 52-53: Handle parsing errors instead of using .unwrap()

Using .unwrap() on b.parse() can cause the program to panic if the parsing fails. It's better to handle the potential error to make the code more robust.

Here's how you can handle the parsing errors:

-             // TODO: error handling
-             .map(|b| b.parse().unwrap())
-             .collect();
+             .map(|b| {
+                 b.parse().map_err(|e| anyhow!("Failed to parse address {}: {}", b, e))
+             })
+             .collect::<Result<Vec<_>>>()?;

This modification propagates the parsing errors properly, allowing the calling function to handle them as needed.

packages/ciphernode/p2p/src/p2p.rs (3)

12-12: Correct the typo in the documentation comment.

In the documentation comment, 'EVentBus' should be 'EventBus'.

---

Line range hint 65-72: Avoid using unwrap() inside asynchronous tasks.

Using unwrap() inside tokio::spawn can cause the task to panic if an error occurs. To prevent unexpected panics, handle the error explicitly.

Apply this diff to handle the potential error:

 tokio::spawn(async move { 
-    libp2p.start().await.unwrap() 
+    if let Err(e) = libp2p.start().await {
+        error!("Error starting libp2p: {:?}", e);
+        // Consider additional error handling here
+    }
 });

---

105-105: Consider adjusting the logging level from trace! to debug!.

The trace! macro is used for very detailed diagnostic information. If these messages are intended for standard debugging, using debug! may be more appropriate to avoid excessive verbosity in the logs.

Also applies to: 111-111

packages/ciphernode/p2p/src/libp2p_router.rs (4)

Line range hint 114-129: Avoid using .unwrap() without checking for None

The method start contains multiple instances of .unwrap() on self.swarm and self.topic. Using .unwrap() can lead to a panic if these options are None. It's safer to handle the None case explicitly.

Consider refactoring to handle None values gracefully:

pub async fn start(&mut self) -> Result<(), Box<dyn Error + Send + Sync + 'static>> {
    if let (Some(swarm), Some(topic)) = (self.swarm.as_mut(), self.topic.as_ref()) {
        swarm.listen_on("/ip4/0.0.0.0/udp/0/quic-v1".parse()?)?;
        swarm.listen_on("/ip4/0.0.0.0/tcp/0".parse()?)?;
        loop {
            select! {
                Some(line) = self.cmd_rx.recv() => {
                    if let Err(e) = swarm.behaviour_mut().gossipsub.publish(topic.clone(), line) {
                        error!(error = ?e, "Error publishing line to swarm");
                    }
                }
                event = swarm.select_next_some() => match event {
                    // ... rest of the event handling ...
                }
            }
        }
    } else {
        return Err("Swarm or topic not initialized".into());
    }
}

---

Line range hint 136-158: Consider adjusting log levels for better visibility

Currently, events like peer discovery and message reception are logged using trace!. Depending on your logging strategy, you might want to use debug! or info! to make these logs more visible during normal operation.

---

114-116: Enhance documentation for the start method

The comment /// Listen on the default multiaddr is brief. Expanding the documentation can help other developers understand the method's purpose and usage.

Consider updating the documentation:

/// Starts the `EnclaveRouter`, listening on the default multiaddresses and processing incoming commands and swarm events.
/// 
/// This method sets up listeners for both QUIC and TCP transports,
/// handles publishing messages received from `cmd_rx`,
/// and processes various swarm events such as new peers discovered via mDNS.
/// 
/// # Errors
///
/// Returns an error if the swarm fails to listen on the specified addresses or if message handling encounters an issue.

---

Line range hint 114-129: Use specific error types instead of Box<dyn Error>

Returning Box<dyn Error + Send + Sync + 'static> erases the concrete error types, making error handling less precise. Consider defining a custom error type or using a specific error enum to improve clarity.

Define a custom error enum:

#[derive(Debug, thiserror::Error)]
pub enum EnclaveRouterError {
    #[error("Swarm or topic not initialized")]
    Uninitialized,
    #[error("IO error")]
    Io(#[from] io::Error),
    #[error("Gossipsub error")]
    Gossipsub(#[from] gossipsub::error::PublishError),
    // ... other error variants ...
}

Update the method signature:

pub async fn start(&mut self) -> Result<(), EnclaveRouterError> {
    // ... method implementation ...
}

Configuration used: CodeRabbit UI
Review profile: CHILL

Configuration used: CodeRabbit UI
Review profile: CHILL